Enforce chroot-like security in fdo.upload owner FSIM using os.Root #163
Conversation
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
2 times, most recently
from
acd3205 to
c49848a
Compare
|
@ben-krieger ptal |
|
Is |
The proposed changes always copy the contents from the temp file to the final destination so it will work across filesystems. However, always copying the contents from the temp file to the final destination is sub-optimal for temp files created in the same filesystem. I like the idea of creating temporary files to upload the content and only create the destination file if the upload was successful so I think the better approach here is to maintain the |
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
from
c49848a to
0fb3899
Compare
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
2 times, most recently
from
c1c38c8 to
a7e2026
Compare
|
@mmartinv can you rebase? @ben-krieger can you take a look? |
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
from
a7e2026 to
904702b
Compare
fsim/upload_owner.go
Outdated
| return false, false, fmt.Errorf("path traversal detected in rename: %q", u.Rename) | ||
| } | ||
|
|
||
| // Backup existing file if present |
There was a problem hiding this comment.
This is potentially nice behavior not to clobber the file, but it's also unconfigurable and undocumented.
Following the principle of least surprise, I think you either overwrite or fail, not backup-and-overwrite.
I believe that the best API decision to reduce user error would be fail-if-existing with an optional configurable function for what to do on conflict.
Another clear API win would be to change Rename from a string to a func(string) string so that users can make every upload timestamped by upload time.
There was a problem hiding this comment.
I basically agree on everything so I did the following changes:
- I changed
Renamefrom astringto afunc(string) stringso now it's possible to define a function to rename the uploaded files. - Fail if the uploaded file already exists and define the
Overwritevariable (default tofalse) to change the behavior if desired.
Apart from that I also changed a bit the behavior of how files are uploaded: Now if the uploads dir is /var/lib/uploads and the file uploaded from the device is /etc/hosts any parent dir will be created relative to the uploads dir and the file will be uploaded to /var/lib/uploads/etc/hosts. I thought that this change alongside #173 will allow us to make per device independent uploads /var/lib/uploads/${device_cert_serial_id}/etc/hosts. If you think there's a better approach just say it and I will try to implement it :)
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
from
904702b to
150ea7f
Compare
Replace direct filesystem operations with os.OpenRoot-based security to prevent path traversal attacks in the upload finalize function. The previous implementation was vulnerable to malicious filenames that could escape the upload directory using path traversal sequences. Key changes: - Change UploadRequest.Rename from string to func(string) string for flexible filename transformation - Add UploadRequest.Overwrite flag to control file replacement behavior - Use os.OpenRoot to create a sandboxed filesystem rooted at u.Dir, preventing all operations from escaping the upload directory - Add platform-specific sameFilesystem() helpers to detect when temp file and destination are on the same filesystem (fs_unix.go, fs_windows.go, fs_other.go) - Use efficient os.Rename when on same filesystem, fall back to io.Copy for cross-filesystem moves - Auto-create parent directories with MkdirAll - Prevent overwriting existing files unless Overwrite flag is set Closes fido-device-onboard#69 Signed-off-by: Miguel Martín <[email protected]>
Add comprehensive test coverage for the os.OpenRoot-based security implementation in the fdo.upload owner module: - TestUploadRequestPathTraversalPrevention: Verifies multiple path traversal attack vectors (../, absolute paths, embedded sequences) are blocked by os.OpenRoot - TestUploadRequestNormalFlow: Validates standard upload with proper file placement and content verification - TestUploadRequestDefaultRename: Confirms default Rename function strips leading slashes from device-provided paths - TestUploadRequestSHA384Mismatch: Ensures upload fails when SHA-384 checksum validation fails - TestUploadRequestSameFilesystemRename: Verifies rename optimization when temp and destination are on same filesystem Also updates existing tests to use Overwrite flag and octal literals. Related to fido-device-onboard#69 Signed-off-by: Miguel Martín <[email protected]>
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
from
150ea7f to
bf4a470
Compare
3 participants
Refactor fdo.upload owner to use os.OpenRoot and prevent path traversal
Replace direct filesystem operations with os.OpenRoot-based security to
prevent path traversal attacks in the upload finalize function. The
previous implementation was vulnerable to malicious filenames that could
escape the upload directory using path traversal sequences.
Key changes:
flexible filename transformation
preventing all operations from escaping the upload directory
file and destination are on the same filesystem (fs_unix.go,
fs_windows.go, fs_other.go)
io.Copy for cross-filesystem moves
Closes #69